Should we use the modified check here as well? Would probably work just as well as the IsTrue check

I didn't because we knew it always would be and this limited the scope of stored, I can go either way though. The alternative would be:

stored := nodeClaim.DeepCopy()
if nodeClaim.StatusConditions().SetTrue(v1.ConditionTypeDrained) {
  // remaining logic
}

I don't really think it tangibly matters -- good either way, just thought it was odd that we didn't keep consistent style here

Maybe throw a comment over this to explain why it's coded this way? I found myself guessing about it again as I was reading through it -- had to re-read the context to understand that this is because of the patch

Is there a way to cleanup some of this logic so that it's not so nested -- comments might also help too -- the fact that we fall-through when volumes are detached and are able to get to the bottom of the function (we don't requeue) is a bit confusing IMO

I've messed around with a few different ways of structuring this, but this ends up being the most readable IMO. I will add some comments to call out the fall-through behavior. FWIW, this does clean up in my larger refactor PR.

Why did you add the fakeClock here -- doesn't look like it is being used anywhere in this testing?

It's passed down to the terminator and was causing nil panics. We could probably scope it down and just pass it there, but I don't mind keeping it test global so it's obvious to use the fake clock passed to the terminator rather than a new one in any future tests that use it.

Maybe throw a comment over this to explain why it's coded this way? I found myself guessing about it again as I was reading through it -- had to re-read the context to understand that this is because of the patch

Given how large this volume attachment code is now, what do you think about moving this into a separate function?

I've been resistant to that because I find the requeue logic to be significantly harder to follow as soon as you move it into a separate function - I had a similar issue when I tried to structure this as subreconcilers. I had been punting those architectural changes for my larger refactor PR. If you don't feel strongly about it I'd rather punt for that, since I think the longer reconcile function is a better than obfuscating the control flow, but let me know what you think.

We don't seem to handle the NotFound error that we get back from this EnsureTerminated call in the same way as we do elsewhere here

These are two functions that I think could be generally useful and I would consider putting in operatorpkg

Good call, I've opened a PR: awslabs/operatorpkg#130